fix: Address memory leaks in API provider clients and Bedrock handler cache #4239

kiwina · 2025-06-02T07:01:34Z

PR for API Provider Client Disposal (api_provider_clients) and Bedrock Handler Cache (bedrock_699)

This PR addresses two potential memory leaks:

Undisposed SDK clients in various API provider handlers, as identified in leak-report/likely/api_provider_clients.md.
Unbounded growth of previousCachePointPlacements in AwsBedrockHandler, as identified in leak-report/likely/bedrock_699.md.

Closes: #4238
Closes: #4240

Description

1. API Provider Clients:
API provider handlers (e.g., AnthropicHandler, OpenAIHandler) create SDK client instances but lacked explicit dispose() methods. The Task class, which holds these handlers, also did not explicitly call for their disposal. This could lead to resource leaks. This patch introduces a dispose() method to the BaseProvider class (which is called reflectively by Task) and ensures Task.abortTask() and Task.dispose() call this.api?.dispose().

2. Bedrock Handler Cache:
The AwsBedrockHandler class uses an instance property previousCachePointPlacements to store cache point information. Entries were added but never removed. This patch overrides the dispose method in AwsBedrockHandler to clear this map and calls super.dispose().

Related Bug Reports

API Provider Clients: fix: Potentially Undisposed SDK Clients in API Providers (api_provider_clients) #4238
Bedrock Handler Cache: fix: Unbounded map growth in AwsBedrockHandler (bedrock_699) #4240

Changes

Modified src/api/providers/base-provider.ts:
- Added a dispose() method that reflectively attempts to call common cleanup methods on a this.client property.
Modified src/api/index.ts:
- Added dispose?: () => void to the ApiHandler interface.
Modified src/core/task/Task.ts:
- Added this.api?.dispose?.() to abortTask() and the new dispose() methods.
Modified src/api/providers/bedrock.ts:
- Overrode the dispose() method in AwsBedrockHandler to clear this.previousCachePointPlacements and call super.dispose().
Modified src/api/providers/vscode-lm.ts:
- Added override keyword to countTokens method.

How to Test

For API Provider Clients:

Create and run tasks using various API providers.
Abort tasks or simulate scenarios where tasks might be disposed of prematurely.
Monitor resource usage (e.g., network connections, memory) to verify that SDK clients are being cleaned up.
Check console logs for dispose messages from BaseProvider if client cleanup methods are found.

For Bedrock Handler Cache:

Create and run multiple tasks using the AWS Bedrock provider with prompt caching enabled.
Ensure each task uses a different conversationId.
Abort tasks or simulate Task.dispose().
Monitor AwsBedrockHandler or add logging to observe the clearing of previousCachePointPlacements.
Verify memory usage does not grow indefinitely with the number of conversations.

daniel-lxs

Hey @kiwina, I left some suggestions for this one.

Might be worth actually having a proper dispose implementation for each provider.

Curious to hear your thoughts.

daniel-lxs · 2025-06-02T14:25:35Z

src/api/providers/base-provider.ts

+			}
+			// Clear the reference on the instance
+			;(this as any).client = undefined
+		}


The reflection-based approach using (this as any).client and typeof checks might be problematic for several reasons:

Loss of type safety: TypeScript cannot verify that subclasses have a client property

Runtime dependency: Relies on property names and method names that might change

Brittle assumptions: Assumes all clients will have one of three disposal methods (close, destroy, or dispose)

Consider a more type-safe approach:

// Option 1: Make dispose abstract and let each provider implement it export abstract class BaseProvider implements ApiHandler { abstract dispose(): void; } // Option 2: Define a client contract export abstract class BaseProvider implements ApiHandler { protected abstract getClient(): { dispose?: () => void } | undefined; public dispose(): void { const client = this.getClient(); client?.dispose?.(); } }

This would ensure each provider explicitly handles its own cleanup logic with proper type checking.

daniel-lxs · 2025-06-02T14:25:56Z

src/core/task/Task.ts

+
+		// Explicitly call dispose on the api handler again, in case abortTask didn't catch it
+		// or if dispose is called directly without abortTask.
+		this.api?.dispose?.()


It seems that these are being called more than once.

abortTask(true) is called, which internally calls this.api?.dispose?.()

Then this.api?.dispose?.() is called again immediately after

This could lead to issues if the disposal method isn't idempotent.

daniel-lxs · 2025-06-02T14:27:48Z

src/api/providers/base-provider.ts

+	 * Disposes of any resources held by the provider.
+	 * Attempts common disposal methods on the client if it exists.
+	 */
+	public dispose(): void {


Several providers in the codebase have client properties but don't override dispose():

AnthropicHandler has private client: Anthropic

OpenAiHandler has private client: OpenAI

BaseOpenAiCompatibleProvider has private client: OpenAI

Other OpenAI-compatible providers (Groq, Chutes, etc.)

These will rely on this reflection-based disposal, which:

Won't work for private properties (they're not accessible via reflection)

Assumes the SDK has one of the three 3 methods

As I said before, I think we should probably have each provider implement its own dispose() method to properly clean up its specific resources, otherwise this implementation doesn't actually do much for providers that don't have any of the 3 disposal methods.

Lets go with a dispose method on the provider, like i mentioned on another pr this would streamline the codebase to use it.

I'll try to get it done in the next couple of days

daniel-lxs · 2025-06-02T14:29:54Z

src/core/task/Task.ts


+	// Added new dispose method as per leak report suggestion
+	public dispose(): void {
+		console.log(`[subtasks] disposing task ${this.taskId}.${this.instanceId}`)


Suggested change

console.log(`[subtasks] disposing task ${this.taskId}.${this.instanceId}`)

i think generally we have way to little logging to trace problems. I did mention this before on discord.
we should have generally some better logging to aid in development, if we don't know whats going on it's has to fix something.

daniel-lxs · 2025-06-02T14:34:40Z

src/core/task/Task.ts

 	}

+	// Added new dispose method as per leak report suggestion
+	public dispose(): void {


This dispose method will directly conflict with your PR #4233.

Yes, there are several PR that touch into the same concept of the Task class needing to dispose of everything it launches.
I could potentially put them all togheter, i belive keeping them seperated for now will give a better insight. merging them is trivial.

kiwina · 2025-06-02T16:54:20Z

Hey @kiwina, I left some suggestions for this one.

Might be worth actually having a proper dispose implementation for each provider.

Curious to hear your thoughts.

Technically speking, i agree, practically speaking they all need to be refactored. My fix takes into account a minimally invasive solution

hannesrudolph · 2025-07-07T20:21:22Z

stale

kiwina added 3 commits June 2, 2025 14:37

fix: auto patch for api_provider_clients

0fa5e34

fix: auto patch for api_provider_clients

5cea844

fix: auto patch for api_provider_clients

8cb5544

kiwina requested review from cte and mrubens as code owners June 2, 2025 07:01

github-project-automation bot moved this to New in Roo Code Roadmap Jun 2, 2025

github-project-automation bot added this to Roo Code Roadmap and Roo Code Roadmap Jun 2, 2025

github-project-automation bot moved this to Triage in Roo Code Roadmap Jun 2, 2025

dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. bug Something isn't working labels Jun 2, 2025

fix: auto patch for bedrock_699

899dc78

kiwina changed the title ~~fix: auto patch for api_provider_clients~~ fix: auto patch for api_provider_clients and bedrock_699 Jun 2, 2025

daniel-lxs moved this from Triage to PR [Needs Prelim Review] in Roo Code Roadmap Jun 2, 2025

daniel-lxs reviewed Jun 2, 2025

View reviewed changes

daniel-lxs moved this from PR [Needs Prelim Review] to PR [Changes Requested] in Roo Code Roadmap Jun 2, 2025

daniel-lxs added the PR - Changes Requested label Jun 2, 2025

daniel-lxs moved this from PR [Changes Requested] to PR [Draft / In Progress] in Roo Code Roadmap Jun 2, 2025

daniel-lxs added PR - Draft / In Progress and removed PR - Changes Requested labels Jun 2, 2025

daniel-lxs changed the title ~~fix: auto patch for api_provider_clients and bedrock_699~~ fix: Address memory leaks in API provider clients and Bedrock handler cache Jun 3, 2025

daniel-lxs marked this pull request as draft June 3, 2025 23:36

hannesrudolph closed this Jul 7, 2025

github-project-automation bot moved this from PR [Draft / In Progress] to Done in Roo Code Roadmap Jul 7, 2025

github-project-automation bot moved this from New to Done in Roo Code Roadmap Jul 7, 2025

fix: Address memory leaks in API provider clients and Bedrock handler cache #4239

fix: Address memory leaks in API provider clients and Bedrock handler cache #4239

Uh oh!

Conversation

kiwina commented Jun 2, 2025 • edited by daniel-lxs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR for API Provider Client Disposal (api_provider_clients) and Bedrock Handler Cache (bedrock_699)

Description

Related Bug Reports

Changes

How to Test

Uh oh!

daniel-lxs left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniel-lxs Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniel-lxs Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

daniel-lxs Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiwina Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

kiwina Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

daniel-lxs Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

kiwina Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

daniel-lxs Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

kiwina Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

kiwina commented Jun 2, 2025

Uh oh!

hannesrudolph commented Jul 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kiwina commented Jun 2, 2025 •

edited by daniel-lxs

Loading

daniel-lxs left a comment •

edited

Loading

daniel-lxs Jun 2, 2025 •

edited

Loading

daniel-lxs Jun 2, 2025 •

edited

Loading